[ISSUE #10529] Fix incorrect queue permissions in gRPC QueryRouteResponse#10531
[ISSUE #10529] Fix incorrect queue permissions in gRPC QueryRouteResponse#10531itxaiohanglover wants to merge 1 commit into
Conversation
…teResponse When the gRPC proxy builds QueryRouteResponse, the permission of each MessageQueue could be incorrect when readQueueNums and writeQueueNums differ. The old implementation grouped queues by permission type (READ, WRITE, READ_WRITE) and assigned increasing queue ids within each group, which preserved the count of each permission but assigned the permission to the wrong queue id. Fix this by deriving the permission per queue id: a queue id is readable if id < readQueueNums and writable if id < writeQueueNums, so READ_WRITE when both conditions hold, WRITE when only writable, and READ when only readable. Add test cases that verify the per-queue-id permission mapping for both readQueueNums < writeQueueNums and the reverse.
RockteMQ-AI
left a comment
RockteMQ-AI
left a comment
There was a problem hiding this comment.
Review by github-manager-bot
Summary
This PR fixes a real bug in RouteActivity#genMessageQueueFromQueueData where queue permissions were assigned to wrong queue IDs in the gRPC QueryRouteResponse. The old code grouped queues by permission type (READ → WRITE → READ_WRITE) and assigned sequential IDs within each group, causing a mismatch between the actual queue ID and its permission.
Findings
-
[Info]
RouteActivity.java:245-297— The refactored logic correctly derives permission per queue ID by iterating from0tomax(readQueueNums, writeQueueNums) - 1. This matches the internal routing logic that builds read/write queues by iterating queue IDs from0. -
[Info]
RouteActivity.java:257-264— The three-way branching (READ_WRITE/WRITE/READ) within the combined readable+writable case is clean and correct. The queue ID directly determines the permission, which is the right approach. -
[Info]
RouteActivity.java:284-286— The no-access branch preserves the originalmax(1, max(writeQueueNums, readQueueNums))behavior. This is fine for backward compatibility, though one could argue thatmax(1, ...)for a no-permission case is already questionable — but that is out of scope for this fix. -
[Info]
RouteActivityTest.java— Good test coverage. The new assertions verify both queue count and the specific permission-to-ID mapping for asymmetric cases (4R/8W and 8R/4W), which would have caught the original bug.
Duplicate PR Notice
Note that PR #10530 by @Kris20030907 targets the same issue (#10529) with a similar fix scope. The maintainers may want to coordinate between the two contributions.
Suggestions
- Minor: The
MessageQueue.newBuilder()...pattern is repeated across all branches. A small helper method likebuildQueue(broker, topic, queueId, permission, topicMessageType)could reduce duplication, but this is a style preference and not blocking.
Verdict
The fix is correct, well-explained, and properly tested. LGTM. 👍
Automated review by github-manager-bot
2 participants
What Changed
Fixed the permission assignment logic in
RouteActivity#genMessageQueueFromQueueDataso that eachMessageQueue's permission is derived from its queue id rather than being assigned by grouped permission counts.Why
When the gRPC proxy builds
QueryRouteResponse, the old implementation calculated the number of read-only, write-only, and read-write queues first, then appended them in groups (READ, WRITE, READ_WRITE) while increasing the queue id. This preserved the count of each permission type but assigned the permission to the wrong queue id.For example, with
readQueueNums = 4,writeQueueNums = 8, andPERM_READ | PERM_WRITE:0..3->READ_WRITE, queue ids4..7->WRITE0..3->WRITE, queue ids4..7->READ_WRITESince
MessageQueue.idis part of the queue identity, the permission must match the actual queue id. The internal route selection logic also builds read/write queues by iterating queue ids from0toreadQueueNums - 1orwriteQueueNums - 1, confirming that the permission should be derived per queue id.How
Instead of grouping by permission type, iterate queue ids from
0tomax(readQueueNums, writeQueueNums) - 1and derive the permission per id:id < readQueueNums && id < writeQueueNums->READ_WRITEid < writeQueueNumsonly ->WRITEid < readQueueNumsonly ->READThe write-only, read-only, and no-access branches remain unchanged.
Test Plan
RouteActivityTest#testGenPartitionFromQueueDatato verify the per-queue-id permission mapping forreadQueueNums = 4, writeQueueNums = 8(ids0..3->READ_WRITE, ids4..7->WRITE)readQueueNums = 8, writeQueueNums = 4(ids0..3->READ_WRITE, ids4..7->READ)maven-compile,license-checker)